-
Notifications
You must be signed in to change notification settings - Fork 12.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement private variables, getter/setters and methods using Symbols #685
Conversation
First off - cool idea. Automatically using symbols for privates, rather than forcing users to generate symbols themselves, sounds like it'd be helpful. It raises a few questions that'd be good to hash out:
|
Did you look at #684 ? The answers are there. In short: 1 and 2: __symbol falls back to a function that returns the parameter if global.Symbol is not found. It's not a polyfill (a full polyfill for Symbol is much more complicated) but it atleast retains the current semantics of TypeScript (pre-symbols). 3: As pointed out in #684 this is definitely not something that should be enabled by default - the perf impact is enormous in all browsers I tested even when not actually using Symbols (i.e., using the fallback). That leads to three situations:
Re: splintering the community with choice - as mentioned in #684, since this only affects the internals of a class and classes can't be split across files (as of now), it's a file-level decision whether to compile with symbols or not. It doesn't have a compatibility impact in that regard. Open classes will be a problem if they're implemented in a way that allows cross-file private member access - then all files that contain the class must be compiled with the same value of the switch. Protected will be a problem if it's implemented using symbols - the symbols must be made accessible from the base class closure to the derived class closure (and preferably not more?), not collide with user-defined members, etc. But yes, all the discussion is in #684 |
I don't see any test changes as part of tihs. I'd like to see how this ends up affecting the emitted code. Thanks! |
} | ||
} | ||
else { | ||
if (symbol.declarations && symbol.declarations.some(function (declaration) { return (declaration.flags & NodeFlags.Private) === NodeFlags.Private; })) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please use arrow notation. i.e. d => d.flags...
also, you just need:
symbol.declarations.some(d => d.flags & SymbolFlags.Private);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've changed it to arrow notation, but I can't remove the equality check because the result of bit-wise and is a number and some wants a callback that returns a bool.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just discovered that ts.forEach is actually the same as Array.prototype.some and not Array.prototype.forEach, so I'll just use that...
Until I add tests, see #684 for the new codegen sample. |
…rty or a static property.
Rebased on latest master. I noticed getPropertyAccessSubstitution is gone now, so I replaced it with two methods that just return true if the property access is for a private or static property respectively. Also reordered the commits so that the codegen is always under the compiler options switch (previously the compiler options switch was added after the new codegen). |
We talked about this at the most recent TypeScript design meeting. Your proposed patch covers a few design points. There was general hesitation around codegen that would do feature detection and then act differently, as there is no precedence for this in TypeScript. Instead, the precedence is around using flags (like outputting to ES5 rather than ES3) that would allow certain patterns through. If we supported symbols as a private implementation, we'd prefer it to not be under feature detection. Instead, let the developer opt in using commandline flags, rather through an ES6 mode or an additional flag. On that note, we debated the pros and cons of using symbols for privates in general as part of an ES6 mode. There's quite a bit we just don't know, yet. Are privates going to be implemented using symbols in ES6? Or will symbols generally not be used for this pattern and largely be used as the known keys, eg [Symbol.iterator]. We don't have enough information to know if this pattern will be common. Until this pattern starts to emerge, we don't have enough to go on to be prescriptive about privates+symbols. The general consensus is to wait and see how classes and symbols get used together. Because of the current concern with perf, it appears that engines need time to catch up, and with that developer practice. It's an interesting idea, and one we should revisit in the future as patterns emerge. |
Agree.
That would just mean changing the __symbol function to use global.Symbol unconditionally and remove the fallback function that returns the original string. But anyway...
Definitely a valid point. User code not using symbols and browsers not optimizing symbols is somewhat of a vicious circle, but I agree that TS doesn't need to take a stance in this. I'm sure Symbols for privates is a quite a ways out before being used in actual code.
Last I checked that got backed out for ES7.
I agree. I primarily did this proposal not because it would be useful, but as an experiment to see what the codegen would look like. I'm myself not convinced that symbols for privates is a good idea either, seeing as it's harder to optimize (although |
Great, sounds like a plan. Closing this pull request. We can revisit later. Thanks for the brainstorming! |
Example implementation for #684
Not for merging, only discussion.
TODO:
Implement compiler flag.Done.Use compiler flag to selectively turn on new codegen.Done.Handle static and instance members with the same name.Done.